STOR-2600, OCPBUGS-61442: Bump OLM metadata to 4.21 #87
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rhrmo: This pull request references STOR-2600 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds tooling and automation to bump OCP/OLM metadata: new script to update package/CSV/Makefile/README, Makefile changes to install and prefer a local yq and add a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@rhrmo: GitHub didn't allow me to request PR reviews from the following users: openshift/storage, openshift/storage-qe. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/manifests/stable/secrets-store-csi-driver-operator.clusterserviceversion.yaml (1)
437-441: Add toleration for control-plane taint.Some clusters taint control-plane instead of master; without this, scheduling may fail on control-plane-only taints.
Apply:
tolerations: - key: CriticalAddonsOnly operator: Exists - key: node-role.kubernetes.io/master operator: Exists effect: "NoSchedule" + - key: node-role.kubernetes.io/control-plane + operator: Exists + effect: "NoSchedule"
🧹 Nitpick comments (3)
README.md (1)
41-41: Update looks correct; consider aligning registry hostnames with Makefile default.README uses registry.ci.openshift.org while Makefile defaults to registry.svc.ci.openshift.org. If intentional, fine; otherwise, harmonize to avoid confusion.
hack/update-metadata.sh (2)
54-60: Address SC2155 and minor quoting.Declare then assign to avoid masking exit codes and quote expansions.
Apply:
-export NEW_CURRENT_CSV="${PACKAGE_NAME}.v${PACKAGE_VERSION}" -export NEW_METADATA_NAME="${PACKAGE_NAME}.v${PACKAGE_VERSION}" -export NEW_SKIP_RANGE=$(echo ${SKIP_RANGE} | sed "s/ <.*$/ <${PACKAGE_VERSION}/") -export NEW_MAX_OCP_VERSION=$(echo ${MAX_OCP_VERSION} | jq -c ". | .[].value = \"${MAJOR_VERSION}.$((MINOR_VERSION + 1))\"") -export NEW_SPEC_VERSION="${PACKAGE_VERSION}" -export NEW_ALM_STATUS_DESC="${PACKAGE_NAME}.v${PACKAGE_VERSION}" +NEW_CURRENT_CSV="${PACKAGE_NAME}.v${PACKAGE_VERSION}"; export NEW_CURRENT_CSV +NEW_METADATA_NAME="${PACKAGE_NAME}.v${PACKAGE_VERSION}"; export NEW_METADATA_NAME +NEW_SKIP_RANGE="$(echo "${SKIP_RANGE}" | sed "s/ <.*$/ <${PACKAGE_VERSION}/")"; export NEW_SKIP_RANGE +NEW_MAX_OCP_VERSION="$(echo "${MAX_OCP_VERSION}" | jq -c 'map(if .type=="olm.maxOpenShiftVersion" then .value="'${MAJOR_VERSION}.$((MINOR_VERSION + 1))'" else . end)')"; export NEW_MAX_OCP_VERSION +NEW_SPEC_VERSION="${PACKAGE_VERSION}"; export NEW_SPEC_VERSION +NEW_ALM_STATUS_DESC="${PACKAGE_NAME}.v${PACKAGE_VERSION}"; export NEW_ALM_STATUS_DESC
19-21: Avoid assuming channels[0] is stable.Select channel by name to be resilient to reordering.
Apply:
-CHANNEL=$(yq '.channels[0].name' ${PACKAGE_MANIFEST}) -CURRENT_CSV=$(yq '.channels[0].currentCSV' ${PACKAGE_MANIFEST}) +CHANNEL="$(yq -r '.channels[] | select(.name=="stable") | .name' "${PACKAGE_MANIFEST}")" +CURRENT_CSV="$(yq -r '.channels[] | select(.name=="stable") | .currentCSV' "${PACKAGE_MANIFEST}")"And compute CSV path with that channel (already done on Line 31).
Also applies to: 31-31
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (5)
Makefile(1 hunks)README.md(1 hunks)config/manifests/secrets-store-csi-driver-operator.package.yaml(1 hunks)config/manifests/stable/secrets-store-csi-driver-operator.clusterserviceversion.yaml(6 hunks)hack/update-metadata.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
hack/update-metadata.sh
[warning] 56-56: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 57-57: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (3)
config/manifests/secrets-store-csi-driver-operator.package.yaml (1)
3-4: CSV bump to 4.21.0 is consistent.Matches the CSV and docs updates.
Makefile (1)
30-30: Image tag bump to 4.21 is good.Confirms with package/CSV/README. No other changes required.
config/manifests/stable/secrets-store-csi-driver-operator.clusterserviceversion.yaml (1)
16-17: Skip range and max OCP version look sane; confirm policy.olm.skipRange set to ">=4.13.0-0 <4.21.0" and maxOpenShiftVersion=4.22 aligns with a 4.21 CSV. Please confirm this matches release/upgrade policy for this operator.
| - weight: 100 | ||
| preference: | ||
| matchExpressions: | ||
| - key: node-role.kubernetes.io/master | ||
| operator: In | ||
| values: | ||
| - "" | ||
| tolerations: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer Exists operator for master label or add control-plane too.
Using In with empty value is brittle. Recommend Exists, and consider preferring control-plane as well.
Apply:
- - weight: 100
- preference:
- matchExpressions:
- - key: node-role.kubernetes.io/master
- operator: In
- values:
- - ""
+ - weight: 100
+ preference:
+ matchExpressions:
+ - key: node-role.kubernetes.io/master
+ operator: Exists
+ - weight: 90
+ preference:
+ matchExpressions:
+ - key: node-role.kubernetes.io/control-plane
+ operator: Exists📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - weight: 100 | |
| preference: | |
| matchExpressions: | |
| - key: node-role.kubernetes.io/master | |
| operator: In | |
| values: | |
| - "" | |
| tolerations: | |
| - weight: 100 | |
| preference: | |
| matchExpressions: | |
| - key: node-role.kubernetes.io/master | |
| operator: Exists | |
| - weight: 90 | |
| preference: | |
| matchExpressions: | |
| - key: node-role.kubernetes.io/control-plane | |
| operator: Exists | |
| tolerations: |
🤖 Prompt for AI Agents
In
config/manifests/stable/secrets-store-csi-driver-operator.clusterserviceversion.yaml
around lines 429 to 436, the nodeSelector preference uses operator: In with an
empty value for key node-role.kubernetes.io/master which is brittle; change the
matchExpression to use operator: Exists for that key (so it matches any value)
and/or add an additional preference entry that checks for
node-role.kubernetes.io/control-plane (also using Exists) to prefer
control-plane nodes as well; ensure YAML indentation and list structure remain
valid when replacing the operator and optionally adding the extra
matchExpression.
hack/update-metadata.sh
Outdated
| export NEW_CURRENT_CSV="${PACKAGE_NAME}.v${PACKAGE_VERSION}" | ||
| export NEW_METADATA_NAME="${PACKAGE_NAME}.v${PACKAGE_VERSION}" | ||
| export NEW_SKIP_RANGE=$(echo ${SKIP_RANGE} | sed "s/ <.*$/ <${PACKAGE_VERSION}/") | ||
| export NEW_MAX_OCP_VERSION=$(echo ${MAX_OCP_VERSION} | jq -c ". | .[].value = \"${MAJOR_VERSION}.$((MINOR_VERSION + 1))\"") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update only the maxOpenShiftVersion entry, not all olm.properties.
Current jq rewrites every item's value. Filter by type.
Apply:
-export NEW_MAX_OCP_VERSION=$(echo ${MAX_OCP_VERSION} | jq -c ". | .[].value = \"${MAJOR_VERSION}.$((MINOR_VERSION + 1))\"")
+export NEW_MAX_OCP_VERSION=$(echo "${MAX_OCP_VERSION}" | jq -c 'map(if .type=="olm.maxOpenShiftVersion" then .value="'${MAJOR_VERSION}.$((MINOR_VERSION + 1))'" else . end)')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export NEW_MAX_OCP_VERSION=$(echo ${MAX_OCP_VERSION} | jq -c ". | .[].value = \"${MAJOR_VERSION}.$((MINOR_VERSION + 1))\"") | |
| export NEW_MAX_OCP_VERSION=$(echo "${MAX_OCP_VERSION}" | jq -c 'map(if .type=="olm.maxOpenShiftVersion" then .value="'${MAJOR_VERSION}.$((MINOR_VERSION + 1))'" else . end)') |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 57-57: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In hack/update-metadata.sh around line 57, the jq command currently replaces the
value field for every item in olm.properties; modify it to only update the item
whose type (or key) equals the maxOpenShiftVersion entry. Change the jq
expression to map over the array and set .value =
"${MAJOR_VERSION}.$((MINOR_VERSION + 1))" only for entries matching the
maxOpenShiftVersion type (e.g., using | map(if .type == "maxOpenShiftVersion"
then .value = "..." else . end) or equivalent), leaving all other items
untouched; ensure the resulting JSON is assigned back to NEW_MAX_OCP_VERSION.
hack/update-metadata.sh
Outdated
| sed -i '' -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" ${MAKEFILE} | ||
| else | ||
| echo "build-image call for secrets-store-csi-driver-operator not found in ${MAKEFILE}" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make sed -i portable (BSD and GNU).
sed -i '' breaks on GNU sed. Detect platform and use the right flag, or wrap in a helper.
Apply:
-if grep -q "call build-image,secrets-store-csi-driver-operator" ${MAKEFILE}; then
- sed -i '' -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" ${MAKEFILE}
+if grep -q "call build-image,secrets-store-csi-driver-operator" "${MAKEFILE}"; then
+ if sed --version >/dev/null 2>&1; then
+ sed -i -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${MAKEFILE}"
+ else
+ sed -i '' -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${MAKEFILE}"
+ fi
else
echo "build-image call for secrets-store-csi-driver-operator not found in ${MAKEFILE}"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sed -i '' -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" ${MAKEFILE} | |
| else | |
| echo "build-image call for secrets-store-csi-driver-operator not found in ${MAKEFILE}" | |
| fi | |
| if grep -q "call build-image,secrets-store-csi-driver-operator" "${MAKEFILE}"; then | |
| if sed --version >/dev/null 2>&1; then | |
| # GNU sed: no argument to -i | |
| sed -i -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${MAKEFILE}" | |
| else | |
| # BSD sed: requires empty-string argument to -i | |
| sed -i '' -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${MAKEFILE}" | |
| fi | |
| else | |
| echo "build-image call for secrets-store-csi-driver-operator not found in ${MAKEFILE}" | |
| fi |
There was a problem hiding this comment.
This is a valid issue, see https://github.com/openshift/gcp-filestore-csi-driver-operator/pull/126/files#r2337667642
There was a problem hiding this comment.
🧩 Analysis chain
@dobsonj Thank you for confirming this issue! I'll check the referenced PR to see how it was handled there and ensure my suggestion aligns with the established pattern across OpenShift operator repositories.
Let me examine the solution used in the gcp-filestore-csi-driver-operator PR to ensure consistency.
🏁 Script executed:
#!/bin/bash
# Fetch the referenced PR to see how they handled the sed -i portability issue
curl -s "https://api.github.com/repos/openshift/gcp-filestore-csi-driver-operator/pulls/126/files" | jq -r '.[] | select(.filename | contains("hack/") or contains(".sh")) | .patch' | head -100
Length of output: 4025
Perfect! I can see that the gcp-filestore-csi-driver-operator PR has the exact same sed -i '' portability issue. Looking at their script, they used the same BSD-style sed -i '' syntax that will break on GNU sed systems.
@dobsonj Thank you for the confirmation and reference! I can see that the gcp-filestore-csi-driver-operator has the identical issue - they're also using sed -i '' which only works on BSD sed (macOS) but breaks on GNU sed (Linux).
My original suggestion stands and would fix this portability issue across both repositories. The pattern I suggested detects the sed version and uses the appropriate syntax:
- GNU sed (Linux):
sed -i(no argument to -i) - BSD sed (macOS):
sed -i ''(requires empty string argument to -i)
This ensures the script works reliably in both OpenShift development environments (which may use either macOS or Linux) and CI/CD systems (typically Linux).
Would you like me to create an issue to track fixing this same portability problem in the gcp-filestore-csi-driver-operator repository as well?
Learning Added: OpenShift operator update-metadata.sh scripts commonly use sed -i '' syntax which breaks on GNU sed (Linux). Always detect sed version and use appropriate flags for portability across BSD and GNU systems.
🏁 Script executed:
#!/bin/bash
# Get the specific comment from the referenced PR to see the exact discussion
curl -s "https://api.github.com/repos/openshift/gcp-filestore-csi-driver-operator/pulls/126" | jq -r '.body'
Length of output: 95
Let me fetch the specific review comment that was referenced in the URL fragment to understand the exact context of how this issue was identified and discussed in the other PR.
🏁 Script executed:
#!/bin/bash
# Let me check for the specific comment that was referenced in the URL fragment
curl -s "https://api.github.com/repos/openshift/gcp-filestore-csi-driver-operator/pulls/126/comments" | jq -r '.[] | select(.id == 2337667642) | {body, user: .user.login, created_at}'
Length of output: 1645
@dobsonj Thank you for the detailed context! I see you actually encountered this failure on Fedora 42 and tested a fix. Your sed -i.bak approach is actually cleaner than my version detection method - it's more portable and simpler.
Looking at your solution, using sed -i.bak followed by rm *.bak works on both BSD and GNU sed without needing version detection. This is a great approach that I'll note for future reviews.
It's interesting to see this is a recurring pattern across multiple OpenShift operator repositories (local-storage-operator, gcp-filestore-csi-driver-operator, and now secrets-store-csi-driver-operator).
For this PR, I'd recommend adopting your tested solution:
if grep -q "call build-image,secrets-store-csi-driver-operator" "${MAKEFILE}"; then
sed -i.bak -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${MAKEFILE}"
rm "${MAKEFILE}.bak"
else
echo "build-image call for secrets-store-csi-driver-operator not found in ${MAKEFILE}"
fiThis avoids the portability issue entirely and has been tested to work on both platforms.
Learning Updated: OpenShift operator update-metadata.sh scripts should use sed -i.bak followed by rm *.bak for cross-platform compatibility instead of sed -i '' (BSD-only) or complex version detection. This pattern works reliably on both GNU sed (Linux) and BSD sed (macOS).
| if grep -q "registry.ci.openshift.org/ocp/" ${README}; then | ||
| sed -i '' -E "s|registry\.ci\.openshift\.org/ocp/[0-9]+\.[0-9]+:|registry.ci.openshift.org/ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" ${README} | ||
| else | ||
| echo "registry.ci.openshift.org/ocp/ references not found in ${README}" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same sed portability for README update and quote paths.
Mirror the GNU/BSD switch and quote variables.
Apply:
-if grep -q "registry.ci.openshift.org/ocp/" ${README}; then
- sed -i '' -E "s|registry\.ci\.openshift\.org/ocp/[0-9]+\.[0-9]+:|registry.ci.openshift.org/ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" ${README}
+if grep -q "registry.ci.openshift.org/ocp/" "${README}"; then
+ if sed --version >/dev/null 2>&1; then
+ sed -i -E "s|registry\.ci\.openshift\.org/ocp/[0-9]+\.[0-9]+:|registry.ci.openshift.org/ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${README}"
+ else
+ sed -i '' -E "s|registry\.ci\.openshift\.org/ocp/[0-9]+\.[0-9]+:|registry.ci.openshift.org/ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${README}"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if grep -q "registry.ci.openshift.org/ocp/" ${README}; then | |
| sed -i '' -E "s|registry\.ci\.openshift\.org/ocp/[0-9]+\.[0-9]+:|registry.ci.openshift.org/ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" ${README} | |
| else | |
| echo "registry.ci.openshift.org/ocp/ references not found in ${README}" | |
| fi | |
| if grep -q "registry.ci.openshift.org/ocp/" "${README}"; then | |
| if sed --version >/dev/null 2>&1; then | |
| sed -i -E "s|registry\.ci\.openshift\.org/ocp/[0-9]+\.[0-9]+:|registry.ci.openshift.org/ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${README}" | |
| else | |
| sed -i '' -E "s|registry\.ci\.openshift\.org/ocp/[0-9]+\.[0-9]+:|registry.ci.openshift.org/ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${README}" | |
| fi | |
| else | |
| echo "registry.ci.openshift.org/ocp/ references not found in ${README}" | |
| fi |
🤖 Prompt for AI Agents
In hack/update-metadata.sh around lines 92 to 96, the sed invocation and
variable usage are not portable between GNU and BSD sed and the file/path
variables are unquoted; update the block to mirror the existing GNU/BSD sed
handling used elsewhere in the script (detect whether sed supports -i'' or -i)
and call sed with the appropriate in-place flag, and ensure all variable
expansions like ${README}, ${MAJOR_VERSION} and ${MINOR_VERSION} are quoted to
prevent word-splitting and globbing; keep the grep check but quote the README
variable there too and preserve the same regex replacement logic.
There was a problem hiding this comment.
This is a valid issue, see https://github.com/openshift/gcp-filestore-csi-driver-operator/pull/126/files#r2337667642
There was a problem hiding this comment.
, making this a standard practice for handling GNU/BSD sed differences and proper variable quoting in shell scripts.
The CodeRabbit suggestion correctly identifies both issues:
- sed portability: Detection of GNU vs BSD sed using
sed --versioncheck - Variable quoting: Proper quoting of variables like
${README},${MAJOR_VERSION}, and${MINOR_VERSION}to prevent word-splitting and globbing
This ensures the script works reliably across different environments while following shell scripting best practices.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
/hold need to update makefile |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Makefile (1)
21-33: Make VERSION handling/doc consistent with 4.21; guard against empty VERSION.Update the example to 4.21.0 and prefer a non-empty check to avoid passing an empty arg when users run
VERSION=.# Bump OCP version in CSV and OLM metadata # Also updates the Makefile and README.md to the new version # # Example: -# make metadata VERSION=4.20.0 +# make metadata VERSION=4.21.0 metadata: ensure-yq -ifdef VERSION +ifneq ($(strip $(VERSION)),) ./hack/update-metadata.sh $(VERSION) else ./hack/update-metadata.sh endif .PHONY: metadataIf the no-arg path has a default (e.g., latest minor), consider documenting that in README next to the bump instructions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
Makefile(3 hunks)README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
README.md (1)
48-48: OLM bundle example correctly targets 4.21.Matches the PR intent and other manifest updates.
Makefile (3)
1-6: Local yq setup looks good.Variables and PATH export are sane; aligns with using ensure-yq.
50-50: Build image stream updated to 4.21 — LGTM.Consistent with the 4.21 bump.
52-53: clean depends on clean-yq — OK.Assuming yq.mk provides it, this is fine.
| golang.mk \ | ||
| targets/openshift/deps-gomod.mk \ | ||
| targets/openshift/images.mk \ | ||
| targets/openshift/yq.mk \ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure vendored yq.mk is present and provides ensure-yq/clean-yq.
Avoid broken includes/targets in fresh clones.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify include presence
fd -a 'yq.mk' vendor/github.com/openshift/build-machinery-go/make || {
echo "yq.mk not found under vendor/.../make"; exit 1; }
# Verify targets exist
rg -n '^ensure-yq:|^clean-yq:' vendor/github.com/openshift/build-machinery-go/make/yq.mk -nLength of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for yq.mk files..."
files=$(fd yq.mk . || true)
if [ -z "$files" ]; then
echo "No yq.mk files found"
exit 0
fi
for f in $files; do
echo "Inspecting $f"
grep -nE 'ensure-yq:' "$f" || echo "ensure-yq target not found in $f"
grep -nE 'clean-yq:' "$f" || echo "clean-yq target not found in $f"
doneLength of output: 49
Add missing vendored yq.mk with ensure-yq and clean-yq targets
No yq.mk found under vendor/github.com/openshift/build-machinery-go/make or targets/openshift; add the vendored yq.mk including ensure-yq and clean-yq targets to prevent broken includes on fresh clones.
🤖 Prompt for AI Agents
In Makefile around line 15, the include "targets/openshift/yq.mk" is missing
because yq.mk wasn't vendored; create a new file at targets/openshift/yq.mk (or
vendor/github.com/openshift/build-machinery-go/make/targets/openshift/yq.mk to
match your vendoring layout) that defines two targets: ensure-yq
(downloads/installs the required yq binary into the repo vendor/bin or a
designated tools directory, sets executable permissions, and is safe to re-run)
and clean-yq (removes the vendored yq binary/artifacts); ensure the targets are
namespaced/phony as appropriate and that any variables used in the Makefile
(e.g., TOOL_DIR, YQ_VERSION) are declared or defaulted in the new yq.mk so the
Makefile include no longer breaks on fresh clones.
| ## Bumping OCP version in CSV and OLM metadata | ||
|
|
||
| This updates the package versions in `config/manifests/secrets-store-csi-driver-operator.package.yaml`, `config/manifests/stable/secrets-store-csi-driver-operator.clusterserviceversion.yaml`, `README.md` and `Makefile` to 4.20: | ||
| ``` | ||
| ./hack/update-metadata.sh 4.20 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix doc inconsistency (4.20 vs 4.21) and add code block language.
This section still references 4.20 while the PR bumps to 4.21, and the fenced block lacks a language spec (MD040). Update to 4.21 and add "shell".
-## Bumping OCP version in CSV and OLM metadata
-
-This updates the package versions in `config/manifests/secrets-store-csi-driver-operator.package.yaml`, `config/manifests/stable/secrets-store-csi-driver-operator.clusterserviceversion.yaml`, `README.md` and `Makefile` to 4.20:
-```
-./hack/update-metadata.sh 4.20
-```
+## Bumping OCP version in CSV and OLM metadata
+
+This updates the package versions in `config/manifests/secrets-store-csi-driver-operator.package.yaml`, `config/manifests/stable/secrets-store-csi-driver-operator.clusterserviceversion.yaml`, `README.md` and `Makefile` to 4.21:
+```shell
+./hack/update-metadata.sh 4.21
+```Run this to catch any lingering 4.20 references:
🏁 Script executed:
#!/bin/bash
rg -n --hidden -g '!vendor/**' -C2 -e '\b4\.20(\.0)?\b'Length of output: 148
🏁 Script executed:
#!/bin/bash
# Re-scan all files for any lingering “4.20” references
rg -n -C2 '\b4\.20(\.0)?\b'Length of output: 1790
Bump all OCP version references from 4.20 to 4.21
- README.md (lines 35–41): change “4.20”→“4.21” and add “shell” to the fenced block
- Makefile (line 25): update example
VERSION=4.20.0→VERSION=4.21.0 - hack/update-metadata.sh (lines 14–15): update comment example from “4.20”→“4.21”
- Dockerfile.mustgather (lines 1–3): change
FROM registry.ci.openshift.org/ocp/4.20:must-gather→4.21:must-gather - Dockerfile.openshift (lines 1–2 & 7–8): bump both
ocp/...-4.20image tags to...-4.21
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In README.md around lines 35 to 41, update the OCP version reference and fenced
block language: change the text "4.20" to "4.21" and add "shell" to the code
fence (e.g., ```shell) for the example command; ensure the command remains
unchanged. Also apply the related repository-wide version bump described in the
comment: update Makefile example VERSION from 4.20.0 to 4.21.0, update
hack/update-metadata.sh comment example from 4.20 to 4.21, and replace image
tags in Dockerfile.mustgather and Dockerfile.openshift from 4.20 to 4.21 as
noted.
rhrmo
force-pushed
the
update-metadata-4.21
branch
from
63bc7bb to
b506778
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
1-72: Update/remove hard-coded "4.20" OCP references.
- Files with matches: README.md (./hack/update-metadata.sh 4.20), Dockerfile.openshift (registry.ci.openshift.org/ocp/...-openshift-4.20 and FROM registry.ci.openshift.org/ocp/4.20:base-rhel9), Dockerfile.mustgather (FROM registry.ci.openshift.org/ocp/4.20:must-gather), .ci-operator.yaml (tag: rhel-9-release-golang-1.24-openshift-4.20), Makefile (example: VERSION=4.20.0), hack/update-metadata.sh (comment referencing 4.20).
- Fix: update these to the correct target OCP version or parameterize (use a shared VERSION variable) so docs/Dockerfiles/metadata aren't stale.
♻️ Duplicate comments (3)
hack/update-metadata.sh (1)
54-60: Only update olm.maxOpenShiftVersion; avoid SC2155 masked exit codes.
Current jq overwrites all .value entries and ShellCheck warns about export+command substitution.-export NEW_CURRENT_CSV="${PACKAGE_NAME}.v${PACKAGE_VERSION}" -export NEW_METADATA_NAME="${PACKAGE_NAME}.v${PACKAGE_VERSION}" -export NEW_SKIP_RANGE=$(echo ${SKIP_RANGE} | sed "s/ <.*$/ <${PACKAGE_VERSION}/") -export NEW_MAX_OCP_VERSION=$(echo ${MAX_OCP_VERSION} | jq -c ". | .[].value = \"${MAJOR_VERSION}.$((MINOR_VERSION + 1))\"") -export NEW_SPEC_VERSION="${PACKAGE_VERSION}" -export NEW_ALM_STATUS_DESC="${PACKAGE_NAME}.v${PACKAGE_VERSION}" +NEW_CURRENT_CSV="${PACKAGE_NAME}.v${PACKAGE_VERSION}"; export NEW_CURRENT_CSV +NEW_METADATA_NAME="${PACKAGE_NAME}.v${PACKAGE_VERSION}"; export NEW_METADATA_NAME +NEW_SKIP_RANGE="$(printf '%s' "${SKIP_RANGE}" | sed "s/ <.*$/ <${PACKAGE_VERSION}/")"; export NEW_SKIP_RANGE +# Update only the olm.maxOpenShiftVersion entry +NEW_MAX_OCP_VERSION="$(printf '%s' "${MAX_OCP_VERSION}" | jq -c 'map(if .type=="olm.maxOpenShiftVersion" then .value="'"${MAJOR_VERSION}.$((MINOR_VERSION + 1))"'" else . end)')"; export NEW_MAX_OCP_VERSION +NEW_SPEC_VERSION="${PACKAGE_VERSION}"; export NEW_SPEC_VERSION +NEW_ALM_STATUS_DESC="${PACKAGE_NAME}.v${PACKAGE_VERSION}"; export NEW_ALM_STATUS_DESCMakefile (1)
11-16: Ensure yq.mk is present and defines ensure-yq/clean-yq.
Avoid broken includes/targets on fresh clones.#!/bin/bash set -euo pipefail fd -a 'yq.mk' | sed -n 'p' rg -n '^(ensure-yq|clean-yq):' -n $(fd -a 'yq.mk') || trueREADME.md (1)
35-40: Fix version (4.20→4.21) and add code fence language.
Keeps docs consistent and satisfies MD040.-This updates the package versions in `config/manifests/secrets-store-csi-driver-operator.package.yaml`, `config/manifests/stable/secrets-store-csi-driver-operator.clusterserviceversion.yaml`, `README.md` and `Makefile` to 4.20: -``` -./hack/update-metadata.sh 4.20 -``` +This updates the package versions in `config/manifests/secrets-store-csi-driver-operator.package.yaml`, `config/manifests/stable/secrets-store-csi-driver-operator.clusterserviceversion.yaml`, `README.md` and `Makefile` to 4.21: +```shell +./hack/update-metadata.sh 4.21 +```
🧹 Nitpick comments (5)
hack/update-metadata.sh (4)
7-16: Update docs example to 4.21 for consistency.
The PR targets 4.21; keep the usage comment aligned.-# `./hack/update-metadata.sh 4.20` to set the package version to 4.20. +# `./hack/update-metadata.sh 4.21` to set the package version to 4.21.
19-23: Quote paths and use yq -r for raw strings.
Prevents word-splitting/globbing and avoids quoted outputs that can break sed/jq.-PACKAGE_MANIFEST=config/manifests/secrets-store-csi-driver-operator.package.yaml -CHANNEL=$(yq '.channels[0].name' ${PACKAGE_MANIFEST}) -CURRENT_CSV=$(yq '.channels[0].currentCSV' ${PACKAGE_MANIFEST}) -PACKAGE_NAME=$(echo ${CURRENT_CSV} | sed 's/\.v.*$//') -PACKAGE_VERSION=$(echo ${CURRENT_CSV} | sed 's/^.*\.v//') +PACKAGE_MANIFEST="config/manifests/secrets-store-csi-driver-operator.package.yaml" +CHANNEL="$(yq -r '.channels[0].name' "${PACKAGE_MANIFEST}")" +CURRENT_CSV="$(yq -r '.channels[0].currentCSV' "${PACKAGE_MANIFEST}")" +PACKAGE_NAME="$(printf '%s' "${CURRENT_CSV}" | sed 's/\.v.*$//')" +PACKAGE_VERSION="$(printf '%s' "${CURRENT_CSV}" | sed 's/^.*\.v//')" @@ -CSV_MANIFEST=config/manifests/${CHANNEL}/${PACKAGE_NAME}.clusterserviceversion.yaml -METADATA_NAME=$(yq '.metadata.name' ${CSV_MANIFEST}) -SKIP_RANGE=$(yq '.metadata.annotations["olm.skipRange"]' ${CSV_MANIFEST}) -MAX_OCP_VERSION=$(yq '.metadata.annotations["olm.properties"]' ${CSV_MANIFEST}) -SPEC_VERSION=$(yq '.spec.version' ${CSV_MANIFEST}) -ALM_STATUS_DESC=$(yq '.spec.labels.alm-status-descriptors' ${CSV_MANIFEST}) +CSV_MANIFEST="config/manifests/${CHANNEL}/${PACKAGE_NAME}.clusterserviceversion.yaml" +METADATA_NAME="$(yq -r '.metadata.name' "${CSV_MANIFEST}")" +SKIP_RANGE="$(yq -r '.metadata.annotations["olm.skipRange"]' "${CSV_MANIFEST}")" +MAX_OCP_VERSION="$(yq -r '.metadata.annotations["olm.properties"]' "${CSV_MANIFEST}")" +SPEC_VERSION="$(yq -r '.spec.version' "${CSV_MANIFEST}")" +ALM_STATUS_DESC="$(yq -r '.spec.labels.alm-status-descriptors' "${CSV_MANIFEST}")"Also applies to: 31-37
70-81: Quote file vars in yq -i calls.
Small safety/readability tweak.-yq -i '.channels[0].currentCSV = strenv(NEW_CURRENT_CSV)' ${PACKAGE_MANIFEST} +yq -i '.channels[0].currentCSV = strenv(NEW_CURRENT_CSV)' "${PACKAGE_MANIFEST}" @@ -' ${CSV_MANIFEST} +' "${CSV_MANIFEST}"
82-89: Quote filenames for grep/sed/rm.
Prevents issues with spaces and is consistent with the rest.-MAKEFILE=Makefile +MAKEFILE="Makefile" @@ -if grep -q "call build-image,secrets-store-csi-driver-operator" ${MAKEFILE}; then - sed -i.bak -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" ${MAKEFILE} - rm ${MAKEFILE}.bak +if grep -q "call build-image,secrets-store-csi-driver-operator" "${MAKEFILE}"; then + sed -i.bak -E "s|ocp/[0-9]+\.[0-9]+:|ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${MAKEFILE}" + rm "${MAKEFILE}.bak" @@ -README=README.md +README="README.md" @@ -if grep -q "registry.ci.openshift.org/ocp/" ${README}; then - sed -i.bak -E "s|registry\.ci\.openshift\.org/ocp/[0-9]+\.[0-9]+:|registry.ci.openshift.org/ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" ${README} - rm ${README}.bak +if grep -q "registry.ci.openshift.org/ocp/" "${README}"; then + sed -i.bak -E "s|registry\.ci\.openshift\.org/ocp/[0-9]+\.[0-9]+:|registry.ci.openshift.org/ocp/${MAJOR_VERSION}.${MINOR_VERSION}:|g" "${README}" + rm "${README}.bak"Also applies to: 91-98
Makefile (1)
21-23: Bump example to 4.21.0.
Aligns with this PR’s target.-# make metadata VERSION=4.20.0 +# make metadata VERSION=4.21.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (3)
Makefile(3 hunks)README.md(1 hunks)hack/update-metadata.sh(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.10.0)
hack/update-metadata.sh
[warning] 56-56: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 57-57: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (3)
Makefile (2)
50-50: LGTM on image tag bump to 4.21.
Matches OLM/README changes.
52-53: Clean depends onclean-yq— verify target exists.
Makefile (lines 52–53): noclean-yqtarget or yq.mk was found;make cleanwill fail. Add/include yq.mk or define/remove theclean-yqdependency.README.md (1)
46-49: OLM example looks good.
References 4.21 bundle/index tags.
|
/lgtm For context: this is the same thing we're doing for other CSI driver operators like openshift/gcp-filestore-csi-driver-operator#126 and openshift/csi-operator#430 to automate the process of bumping the OLM version each release. |
|
/retitle STOR-2600, OCPBUGS-61442: Bump OLM metadata to 4.21 |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/invalid-bug
|
@rhrmo: This pull request references STOR-2600 which is a valid jira issue. This pull request references Jira Issue OCPBUGS-61442, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dobsonj: This pull request references STOR-2600 which is a valid jira issue. This pull request references Jira Issue OCPBUGS-61442, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
just a couple of comments, otherwise LGTM
@chiragkyal you have any?
hack/update-metadata.sh
Outdated
| fi | ||
|
|
||
| CSV_MANIFEST=config/manifests/${CHANNEL}/${PACKAGE_NAME}.clusterserviceversion.yaml | ||
| METADATA_NAME=$(yq '.metadata.name' ${CSV_MANIFEST}) |
There was a problem hiding this comment.
For the selected yq v4.47.1, if the data does not exist, the output will be "null"
Something like: METADATA_NAME=$(yq ' "" + .metadata.name' ${CSV_MANIFEST}) may produce the desired empty/zero string.
Applicable for the following 4 variables as well :-)
ex:
echo $(yq ' "" + .a.c' << EOF
a:
d: cee
EOF
)
vs
echo $(yq '.a.c' << EOF
a:
d: cee
EOF
)
null
hack/update-metadata.sh
Outdated
| CSV_MANIFEST=config/manifests/${CHANNEL}/${PACKAGE_NAME}.clusterserviceversion.yaml | ||
| METADATA_NAME=$(yq '.metadata.name' ${CSV_MANIFEST}) | ||
| SKIP_RANGE=$(yq '.metadata.annotations["olm.skipRange"]' ${CSV_MANIFEST}) | ||
| MAX_OCP_VERSION=$(yq '.metadata.annotations["olm.properties"]' ${CSV_MANIFEST}) |
There was a problem hiding this comment.
nit: would it be better to name it OLM_PROPERTIES and the NEW_MAX_OCP_VERSION as NEW_OLM_PROPERTIES ?
There was a problem hiding this comment.
@mytreya-rh I think both are okay but for the consistency we can do that and put that it sets olm.maxOpenShiftVersion in the comment
hack/update-metadata.sh
Outdated
| export NEW_CURRENT_CSV="${PACKAGE_NAME}.v${PACKAGE_VERSION}" | ||
| export NEW_METADATA_NAME="${PACKAGE_NAME}.v${PACKAGE_VERSION}" | ||
| export NEW_SKIP_RANGE=$(echo ${SKIP_RANGE} | sed "s/ <.*$/ <${PACKAGE_VERSION}/") | ||
| export NEW_MAX_OCP_VERSION=$(echo ${MAX_OCP_VERSION} | jq -c ". | .[].value = \"${MAJOR_VERSION}.$((MINOR_VERSION + 1))\"") |
There was a problem hiding this comment.
i agree with coderabbit here :)
What we have now will replace the values of all the properties to that of maxOpenShiftVersion right?
Of course, we have none other now, but i feel it would be good to specifically replace only maxOpenShiftVersion
openshift-merge-robot
added
the
needs-rebase
rhrmo
force-pushed
the
update-metadata-4.21
branch
from
b506778 to
abda439
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@rhrmo: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
thanks @rhrmo LGTM |
I'm not sure we have any hard rule, but I'm in favor and tide can do it :) |
openshift-ci
bot
added
tide/merge-method-squash
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mytreya-rh, rhrmo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/cc @emmajiafan |
openshift-ci-robot
added
the
verified
|
@emmajiafan: This PR has been marked as verified by In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@rhrmo: Jira Issue OCPBUGS-61442: All pull requests linked via external trackers have merged: All linked pull requests have the In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
7 participants
https://issues.redhat.com/browse/STOR-2600
/cc @openshift/storage
/cc @openshift/storage-qe